@topce Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@topce Thank you for signing the Contributor License Agreement!

This PR doesn't fix CVE-2023-24998 by itself, as upgrading merely exposes the new configuration option introduced in commons-fileupload. According to https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457, the option must be manually set in order to avoid security issues.

This PR effectively raises the commons-fileupload baseline to 1.5 as it relies on newly introduced API; as a result, I don't think we can apply this in a maintenance branch like 5.3.x. This support has been removed entirely in Spring Framework 6.0 (see #27423).

I think the most viable options here would be:

• override the CommonsFileUploadSupport.newFileUpload method in a subclass and setting the option there
• use the container-based variant instead

"This PR doesn't fix GHSA-hfrx-6qgj-fp6c by itself, as upgrading merely exposes the new configuration option introduced in commons-fileupload. According to https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457, the option must be manually set in order to avoid security issues."

Agree user still need to set new configuration option in order to avoid DoS

"This PR effectively raises the commons-fileupload baseline to 1.5 as it relies on newly introduced API; as a result, I don't think we can apply this in a maintenance branch like 5.3.x. This support has been removed entirely in Spring Framework 6.0 (see #27423)."

Here not sure , I know that it is removed from 6.0 that why made PR for 5.3.x that is still supported.

"I think the most viable options here would be:

override the CommonsFileUploadSupport.newFileUpload method in a subclass and setting the option there
use the container-based variant instead"

Can you elaborate more here not sure that understand.
I think it is more easy to use new parameter
for example
multipartResolver.setFileCountMax
like before :
multipartResolver.setMaxUploadSize
multipartResolver.setMaxUploadSizePerFile

@topce if we introduce this change, this makes commons-fileupload 1.5 a requirement for all applications. This means that an application upgrading from the current 5.3.27 to the next maintenance version 5.3.28 would be broken and would be required to upgrade to commons-fileupload 1.5. We usually don't introduce such changes in maintenance versions.

• override the CommonsFileUploadSupport.newFileUpload method in a subclass and setting the option there

As described in the reference documentation, you need to create a CommonsMultipartResolver. In this case, one could subclass CommonsMultipartResolver and override its newFileUpload method to set that setFileCountMax option.

• use the container-based variant instead

As for using the standard Servlet mechanism, this is described in the reference documentation - and is the preferred variant anyway.

@bclozel thanks for explanation!

"if we introduce this change, this makes commons-fileupload 1.5 a requirement for all applications. This means that an application upgrading from the current 5.3.27 to the next maintenance version 5.3.28 would be broken and would be required to upgrade to commons-fileupload 1.5. We usually don't introduce such changes in maintenance versions."

Understand but maybe here it is worth to do it because it allow them to address security issue potential DoS attack

"As described in the reference documentation, you need to create a CommonsMultipartResolver. In this case, one could subclass CommonsMultipartResolver and override its newFileUpload method to set that setFileCountMax option."
Yes but this new property is present in latest version of commons-fileupload 1.5 so anyway they need to upgrade to latest version.

"As for using the standard Servlet mechanism, this is described in the reference documentation - and is the preferred variant anyway."
Thank you, for this one I did not know.
Is this option available in spring framework 5.3.x?

@topce Yes, the StandardServletMultipartResolver class has been available since Spring Framework 3.1.

@topce Yes, the StandardServletMultipartResolver class has been available since Spring Framework 3.1.

@bclozel yes but even there you need to migrate commons-fileupload 1.5 if you want to address
potential DoS issue !?
As this is security patch I think it is justify to make commons-fileupload 1.5 a requirement for all applications.

Merging this PR is not enough to secure applications out of the box as the library does not enforce any value by default. Developers will require to manually update their codebase for that.

We will discuss this issue as a team and report back here.

OK
Thanks!

@bclozel
FYI
I fixed it in my code by
upgrading to common-fileupload 1.5
then create class SafeCommonsMultipartResolver that
extends CommonsMultipartResolver add
setFileCount method and override prepareFileUpload

// fix potential DoS attack
// https://security.snyk.io/vuln/SNYK-JAVA-COMMONSFILEUPLOAD-3326457?utm_medium=Partner&utm_source=RedHat&utm_campaign=Code-Ready-Analytics-2020&utm_content=vuln%2FSNYK-JAVA-COMMONSFILEUPLOAD-3326457
public class SafeCommonsMultipartResolver extends CommonsMultipartResolver {

  private long fileCountMax;

  public void setFileCountMax(final long fileCountMax) {
    this.fileCountMax = fileCountMax;
  }

  @Override
  protected FileUpload prepareFileUpload(@Nullable String encoding) {
    FileUpload actualFileUpload = super.prepareFileUpload(encoding);
    actualFileUpload.setFileCountMax(fileCountMax);
    return actualFileUpload;
  }
}

finally I use new class SafeCommonsMultipartResolver instead CommonsMultipartResolver
and limit file count to one because application does not support uploading multiple files

@Bean(name = "multipartResolver")
  public MultipartResolver multipartResolver(EushareConfiguration esConfig) {
    SafeCommonsMultipartResolver multipartResolver = new SafeCommonsMultipartResolver();

    // prevent Dos attack
    // we just upload one file
    multipartResolver.setFileCountMax(1);

    multipartResolver.setMaxUploadSize(esConfig.getMaxSizeAllowedInBytes());
    multipartResolver.setMaxUploadSizePerFile(
        esConfig.getMaxSizeAllowedInBytes());
    return multipartResolver;
  }

@bclozel
In order to fix issue I need to add new class that inherit Spring class
override method add new method
use new class instead of Spring class
call new method
if you accept PR
I would just need to add one line of code :
multipartResolver.setFileCountMax(1);

That was main motivation for this PR

We have decided to keep things as is because there is a workaround for this problem (and manual configuration is required anyway). Also, this support has been removed in the next generation of Framework so this is the opportunity for 5.3.x users to migrate to the standard fileupload support.